Fix: #580 Create ModalCommandPalette #646
Conversation
✅ Deploy Preview for activist-org ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Thank you for the pull request!The activist team will do our best to address your contribution as soon as we can. The following is a checklist for maintainers to make sure this process goes as well as possible. Feel free to address the points below yourself in further commits if you realize that actions are needed :) If you're not already a member of our public Matrix community, please consider joining! We'd suggest using Element as your Matrix client, and definitely join the General and Development rooms once you're in. It'd be great to have you! Maintainer checklist
|
|
Thanks @mattburnett-repo! Do you want to check the errors reported above, or should I get to them? |
I'd be happy to do the i18n ones on review :) |
|
I made the errors. I'll fix them. Should have another commit / PR in maybe an hour. |
|
fixed the i18n errors in the ModalCommandPalette code. the remaining i18n_check error involves code outside of the code for ModalCommandPalette. could you fix that one, please? |
| }, | ||
| }); | ||
|
|
||
| whenever(slash, () => { |
There was a problem hiding this comment.
Quick thing that I'm realizing here @mattburnett-repo is that I didn't do a good job of explaining that slash should maintain its old functionality for this :)
Ultimately activist will have two kinds of search: the slash one that's from the search bar that searches within the current domain for events or organizations or anything if we're not within a sub domain, and then the command palette that should only be triggered by meta_k or ctrl_k.
Would it be possible to:
- Remove the slash functionality from this
- Uncomment those lines in the search bar to show the command palette shortcut indicators
I'll then go through and do some styling. After that we should be good!
There was a problem hiding this comment.
No worries @andrewtavis . We had earlier some discussion about overall CommandPalette functionality and I probably got confused during that discussion.
I've implemented your suggestions and they will appear in the next PR.
There was a problem hiding this comment.
Looking forward to checking out the finalized version later, @mattburnett-repo! Thanks for the quick reaction :) :)
| import { useMagicKeys, whenever } from "@vueuse/core"; | ||
|
|
||
| // TODO: Maybe there is a better / more future-proof solution than userAgent? | ||
| const isMacOS = /Mac/.test(navigator.userAgent); |
There was a problem hiding this comment.
You can use the device check module as is done here 😊 These are also the lines we want to be uncommented :)
There was a problem hiding this comment.
This is as good of a solution as I could think of, and would be fine for now until we realize that it has major drawbacks.
There was a problem hiding this comment.
Got it. The $device.isMacOS (from the template example) doesn't work in a script block. It took me a minute, but I figured out to use useDevice().
There was a problem hiding this comment.
Awesome! Great to hear, @mattburnett-repo!
|
Note to self, do not do Headwind formatting for the Tailwind classes while reviewing as it'll cause crazy merge conflicts 😅 |
There was a problem hiding this comment.
First version is done, @mattburnett-repo! This is really amazing stuff here 😊 There are still some minor things to do that I'll mention in the issue, but I think we're good to merge this in :)
Contributor checklist
Description
This PR addresses Issue #580. It creates a baseline for the component as described in the original issue.
It is only tested on MacOS.
There are some TODO tasks:
│ │ └─ ModalCommandPalette.vue
│ │ ├─ line 18: TODO : Input area is much smaller that tag width.
│ │ ├─ line 37: TODO : Sort out router-link target routes (the to="..." things).
│ │ └─ line 170: TODO : Maybe there is a better / more future-proof solution than userAgent?
Related issue